Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the Makefile build fully customizable #113

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

mmartinv
Copy link
Contributor

@mmartinv mmartinv commented Jan 17, 2023

  • Prefer podman over docker in Makefile builds
  • Make the build fully customizable through environment
    variables.
  • Use CONTROLLER_GEN var to specify which controller-gen
    binary must be used and make sure the correct version is
    installed when using the default one.
  • Make consistent use of parentheses instead of curly braces
    for variable delimitation.
  • Add sandbox writeable paths, needed after bazel clean.

@mmartinv mmartinv requested a review from ahadas as a code owner January 17, 2023 12:18
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 33.93% // Head: 33.93% // No change to project coverage 👍

Coverage data is based on head (612269a) compared to base (5d78636).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #113   +/-   ##
=======================================
  Coverage   33.93%   33.93%           
=======================================
  Files          42       42           
  Lines        8080     8080           
=======================================
  Hits         2742     2742           
  Misses       5121     5121           
  Partials      217      217           
Flag Coverage Δ
unittests 33.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@mmartinv mmartinv force-pushed the makefile-changes branch 2 times, most recently from f23cd8a to 0ab4a00 Compare January 19, 2023 09:05
@mmartinv mmartinv requested a review from nyoxi January 19, 2023 09:08
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@mmartinv mmartinv force-pushed the makefile-changes branch 2 times, most recently from cec0f7f to 12f9f5d Compare January 19, 2023 12:22
@mmartinv mmartinv requested a review from nyoxi January 19, 2023 12:23
Copy link
Member

@nyoxi nyoxi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The only questionable part (from my POV) is the use of shell which docker versus shell which podman. I would prefer to have Podman as default but I am not going to block the PR on this.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one comment about setting CONTAINER_CMD in .bazelrc

.bazelrc Outdated
@@ -1,3 +1,6 @@
# Container Runtime
build --action_env=CONTAINER_CMD=/usr/bin/docker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case both docker and podman are installed this is used to force the use of one of them. This could be useful to test builds with both container runtimes without having to remove podman to test docker.

Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment about indentation, other than that - lgtm
@mnecas can you please take a look at the operator part?

Makefile Outdated Show resolved Hide resolved
@mmartinv mmartinv force-pushed the makefile-changes branch 3 times, most recently from 8ef8e02 to 99c3470 Compare January 26, 2023 09:24
.bazelrc Show resolved Hide resolved
Prefer `podman` over `docker`.

Make the build fully customizable through environment
variables.

Use `CONTROLLER_GEN` var to specify which `controller-gen`
binary must be used and make sure the correct version is
installed when using the default one.

Make consistent use of parentheses instead of curly braces
for variable delimitation.

Add sandbox writeable paths, needed after `bazel clean`.

Signed-off-by: Miguel Martín <mmartinv@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mmartinv mmartinv requested a review from mnecas January 26, 2023 14:36
@ahadas ahadas merged commit 7a8142e into kubev2v:main Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants